-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] workflow support? #26
Conversation
KostyaSha
commented
Oct 26, 2015
- Support Job class (workflow support)
- github-plugin update to 1.14.2
- some failed mock tests ignored
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
should be rebased - it shows all changes, not only for wf |
rebased |
throws InterruptedException, IOException { | ||
GitHubPRTrigger trigger = build.getProject().getTrigger(GitHubPRTrigger.class); | ||
// No triggers in Run class, but we need it | ||
AbstractBuild<?, ?> build = (AbstractBuild<?, ?>) run; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be class cast exception on wf job type
@jglick you are welcome ;) |
@@ -25,7 +28,9 @@ | |||
/** | |||
* Sets pr status for build caused by GitHubPRCause | |||
*/ | |||
public class GitHubPRStatusBuilder extends Builder { | |||
public class GitHubPRStatusBuilder extends Builder implements SimpleBuildStep { | |||
private static final long serialVersionUID = 1L; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be necessary. Build steps are not generally Serializable
. (They may be persisted via XStream but that is not going to care about a serialVersionUID
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mm... i saw plugins with Serialisable Builders... Just wanted to make it UID fixed in case of future changes.
expandedMessage = statusMessage.expandAll(build, listener); | ||
} else { | ||
// Workflow doesn't expand variables with token macro! | ||
expandedMessage = statusMessage.getContent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May need to specifically expand ${GITHUB_PR_COND_REF}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like i should wrote Util that will try expand all possible variables from JobMixIns. That will at least expand ParameterActions.
|
||
content = (String) tokenMacroExpand.invoke(null, build, listener, content, false, macros); | ||
//get private macroses like groovy template ${SCRIPT} if available | ||
if (jenkins.getPlugin("email-ext") != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😠 @lanwen tune to 4 nested at least?
@lanwen Allow 4?
Under question |
Good point. Don't hide fields and attributes when reusing vars. Thats why Oleg Nenashev make all vars final. But you just should create new vars in local context |
List macros = null; | ||
if (jenkins.getPlugin("token-macro") != null) { | ||
// get private macroses like groovy template ${SCRIPT} if available | ||
if (jenkins.getPlugin("email-ext") != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When private macros will be in token macro nested if will disappear, but now it right logic. 3 is too small
@@ -75,38 +89,49 @@ public void createBuilderWithCustomMessage() { | |||
Assert.assertEquals(CUSTOM_MESSAGE, builder.getStatusMessage().getContent()); | |||
} | |||
|
|||
@Ignore("Can't mock workspace") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lanwen you are welcome with suggestion how to mock this test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you as developer should know how to test it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As developer i can ignore them :D But as i see current mock library can't mock final/static methods
Exclude overlapped argument
Any progress on this? :) |
@emanuelez sorry, too busy 🏧 plus i not fully understand what @jglick do with all this multi-branches and workflow specific features vs standard freestyle (non-workflow) jobs. |
@@ -422,7 +424,8 @@ public DescriptorImpl() { | |||
|
|||
@Override | |||
public boolean isApplicable(Item item) { | |||
return item instanceof AbstractProject; | |||
return item instanceof Job && SCMTriggerItem.SCMTriggerItems.asSCMTriggerItem(item) != null | |||
&& item instanceof ParameterizedJobMixIn.ParameterizedJob; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means it's only applicable for parameterized jobs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afair copy-pasted from GH plugin, but work of triggering is based on parameter actions, so probably it right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, makes sense.
@emanuelez don't remember, maybe it works. Do you want try? :) |
public void perform(@Nonnull Run<?, ?> run, @Nonnull FilePath workspace, @Nonnull Launcher launcher, | ||
@Nonnull TaskListener listener) throws InterruptedException, IOException { | ||
// No triggers in Run class, but we need it | ||
final GitHubPRTrigger trigger = JobInfoHelpers.triggerFrom(run.getParent(), GitHubPRTrigger.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for what it's worth, I'd like to see something like that method (and probably the whole class) in core somewhere to deal with the Run/AbstractProject pain.
So I tend to think it's probably not worth worrying about Multibranch - that's so different in terms of how it triggers, gets source, etc, that this whole plugin would probably need a rewrite to work there (though correct me if I'm wrong, @jglick!). It feels like it should work fine with a non-Multibranch Workflow, though. |
What i really want - support all projects (multibranch, non-multibranch (freestyle), workflow). But i'm very confused with all hacks for APIs. If i right understand workflow doesn't work with SCRIPT_TOKENS. It is feature of this plugin to provide groovy templates for GitHub comment posting (if checkboxes must die) and i used email-ext templates, while searching generic way of having groovy scripts with security in jenkins. But workflow as i understand requires variable(?) in it's job definition that will be expanded during wf processing. So it seems that some global wf command that will be able resolve tokens is required. So for now WF will miss such feature. |
if (trigger == null) { | ||
return true; | ||
// silently skip. TODO implement error handler, like in publishers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please show me an example of what you mean "like in the publishers". I am trying to convert the http-request plugin and I have the same question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plugin Publishers has errorHandler, this class is Builder and i forgot to use it also. ErrorHandler is useful only for linear jobs, workflow allows handle errors in DSL directly.
@@ -61,52 +64,55 @@ public GitHubPRBuildStatusPublisher(GitHubPRMessage statusMsg, GHCommitState uns | |||
} | |||
|
|||
@Override | |||
public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws | |||
InterruptedException, IOException { | |||
public void perform(@Nonnull Run<?, ?> run, @Nonnull FilePath workspace, @Nonnull Launcher launcher, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged via 6ef6d12 |